Skip to content

Add BDF export #13435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add BDF export #13435

wants to merge 7 commits into from
+150 −54

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Oct 7, 2025 •
edited
Loading

Now that edfio 0.4.10 added support for BDF, we should support that in MNE as well. The current implementation duplicates a lot of EDF code, which I think is OK for now (I'd prefer to refactor in a follow-up PR).

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025 •
edited
Loading

Any ideas why the ultraslow_pg job is failing?

The failing Windows pip pre job is due to pyvistaqt DeprecationWarnings, so this is unrelated I guess. The Ubuntu pip pre job segfaults, also due to some VTK issue.

@cbrnr cbrnr added the backport-candidate on-merge: backport to maint/1.10 label Oct 7, 2025
@larsoner
Copy link
Member

larsoner commented Oct 7, 2025

The current implementation duplicates a lot of EDF code, which I think is OK for now (I'd prefer to refactor in a follow-up PR).

It looks like _bdf.py and _edf.py only differ by < 10 lines. Can I push a commit just to take care of this now? Seems simpler and cleaner than adding a bunch of lines just to almost immediately remove it

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Sure, but if you want I can also refactor it.

@larsoner
Copy link
Member

larsoner commented Oct 7, 2025

Sure feel free! On Azure this failure does look related:

_____________________ ERROR collecting mne/export/_bdf.py ______________________
mne/export/_bdf.py:13: in <module>
    _check_edfio_installed()
mne/utils/check.py:451: in _check_edfio_installed
    return _soft_import("edfio", "exporting to EDF", strict=strict)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mne/utils/check.py:429: in _soft_import
    raise RuntimeError(
E   RuntimeError: For exporting to EDF to work, the module edfio is needed, but it could not be imported. Use the following installation method appropriate for your environment:
E   
E       pip install edfio
E       conda install -c conda-forge edfio
---------- generated xml file: /home/vsts/work/1/s/junit-results.xml -----------
=========================== short test summary info ============================
ERROR mne/export/_bdf.py - RuntimeError: For exporting to EDF to work, the module edfio is needed, but it could not be imported. Use the following installation method appropriate for your environment:

    pip install edfio
    conda install -c conda-forge edfio

but maybe the refactoring already took care of it. I'll tackle the unrelated errors in #13434

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

I don't understand this error. The new test_export_raw_bdf function has the exact same markers as test_export_raw_edf (i.e., @edfio_mark() and pytest.mark.slowtest in particular). So why is this test collected in the first place?

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Found it and hopefully fixed it.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Looks good, feel free to merge @larsoner!

drammock
drammock reviewed Oct 7, 2025
View reviewed changes
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple minor things

mne/export/_edf_bdf.py
annotations.append(EdfAnnotation(onset, duration, desc))

Edf(
annotations.extend(pad_annotations)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, doing it this way will change the order of the items in the annotations list (previously, all the BAD_ACQ_SKIP` would be at the beginning, now they will be at the end). Does that matter? Or does EDF/BDF not care (or does the writer sort the annots in any case)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it didn't really matter (the docs are not clear or I couldn't find that info). @hofaflo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found it, annotations are sorted before writing to a file:

https://github.com/the-siesta-group/edfio/blob/main/edfio/edf_annotations.py#L86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock left review comments

@larsoner larsoner Awaiting requested review from larsoner larsoner is a code owner

@sappelhoff sappelhoff Awaiting requested review from sappelhoff sappelhoff is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

Assignees
No one assigned
Labels
backport-candidate on-merge: backport to maint/1.10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cbrnr @larsoner @drammock